Add Windows 7 release target by Athemis · Pull Request #11 · Athemis/ELNPack

Athemis · 2025-12-03T14:28:07Z

Summary

Add win7-windows-msvc release targets

Testing

cargo fmt
cargo clippy --all-targets --all-features
cargo test

Notes for reviewers

Summary by CodeRabbit

New Features
- CI matrix expanded for per-target toolchain control, selective target installation, and improved OS-specific packaging/naming (including Windows win7/nightly entries).
Bug Fixes
- Attachment validation now rejects duplicate sanitized names.
- Keywords now deduplicate case-insensitively while preserving first-seen casing.
Documentation
- Enhanced and clarified docs and examples across multiple modules; adjusted doc-test annotations and added examples/guidance.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- we are not exposing any public functions, hence doctests will fail otherwise

coderabbitai · 2025-12-03T14:28:17Z

Walkthrough

Documentation expanded across many modules; keyword normalization added; attachment duplicate-name detection implemented; release workflow matrix extended with per-target toolchain settings, conditional rust-src/install-target steps, and SAFE_REF-driven dynamic packaging. No public API signatures changed.

Changes

Cohort / File(s)	Change Summary
Release workflow & packaging `\.github/workflows/release.yml`	Expanded Rust matrix with per-target `toolchain`, `build_std_flag`, and `install_target`; cache keys include `matrix.toolchain`/`matrix.target`; conditional `rustup target` and `rust-src` installation; cargo invoked as `cargo +<toolchain>` with `matrix.build_std_flag`; SAFE_REF computed per-OS and used in dynamic package dirs and artifact names.
Attachment validation `src/models/attachment.rs`	Added doc comments for `Attachment::new` and `assert_unique_sanitized_names`; implemented duplicate `sanitized_name` detection so `assert_unique_sanitized_names` returns an error on duplicates.
Keyword normalization `src/models/keywords.rs`	Added a private `normalize` method; `Keywords::new` now deduplicates keywords case-insensitively while preserving first-seen casing; public API unchanged.
Documentation additions `src/app/mod.rs`, `src/utils/hash.rs`, `src/utils/sanitize_component.rs`	Expanded doc comments (Errors, Examples, behavior descriptions). Signatures and runtime behavior unchanged.
Doc-example standardization `src/logic/eln.rs`, `src/models/extra_fields.rs`, `src/mvu/mod.rs`, `src/ui/components/extra_fields.rs`	Converted many doc-code fences to `rust,ignore` (or equivalent) to opt out of rustdoc execution; removed some example blocks.
Minor UI syntax modernization `src/ui/components/extra_fields.rs`	Replaced a match/unwrap pattern with a `let-else` early-return for draft retrieval; semantics preserved.
UI docs cleanup `src/ui/mod.rs`	Removed several `# Examples` sections from public method doc comments; no functional changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas requiring focused attention:
- src/models/keywords.rs — verify normalization (case-insensitive dedupe, casing preservation, whitespace behavior).
- src/models/attachment.rs — ensure error type and behaviour for duplicate sanitized_name are correct and tested.
- .github/workflows/release.yml — validate matrix permutations, conditional steps (install_target, build_std_flag, rust-src), SAFE_REF computation, and artifact naming.

Possibly related PRs

Feature/32bit release targets #3 — Overlapping changes to .github/workflows/release.yml (per-target matrix and SAFE_REF-based packaging).
Implement metadata fields #4 — Related edits in src/models/extra_fields.rs and doc-example handling.

Poem

"Engage: docs refined, duplicates kept at bay,
Keywords march in order, bright as day,
Packaging charts a broader starry road,
Small fixes hum where steady systems flowed,
Forward we go — steady, true, and gay."

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the primary objective of adding Windows 7 release targets, clearly summarizing the main change.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The pull request description follows the required template structure with SPDX headers, Summary, Testing checklist, and Notes sections; however, the Testing checklist items are marked as checked despite the PR being in draft status.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature/win7-release-target

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/mvu/mod.rs (1)

337-347: Consider surfacing a specific message for invalid email fields.

In validate_for_save, URL, number, and integer errors get tailored user messages, while the "invalid_email" case falls through to the generic "Field '{label}' is invalid.". For consistency and clearer feedback, I recommend adding an explicit branch for "invalid_email" (and corresponding tests alongside the URL/number/integer cases).

Also applies to: 483-507

🧹 Nitpick comments (5)

src/ui/components/extra_fields.rs (2)

45-56: Extra‑fields UI helpers are now well-documented and easier to reason about.

The new Rustdoc on ExtraFieldsModel accessors, helpers like split_multi, name_conflict, apply_draft_to_field, and the test utilities significantly clarifies the MVU flow and invariants while keeping UI code side‑effect free. Using rust,ignore for sketch-like examples is appropriate here. Nicely done.

Also applies to: 78-84, 95-101, 137-142, 162-171, 187-201, 318-323, 389-397, 415-423, 443-451, 463-471, 487-502, 607-615, 1210-1213, 1229-1236, 1255-1258, 1267-1271, 1288-1325, 1534-1543, 1567-1573, 1735-1753, 1765-1788, 1868-1877

1365-1421: render_field_modal’s early‑return guard is a sensible defensive improvement.

The let Some(draft) = model.modal_draft.clone() else { return; }; guard ensures the modal is only rendered when a draft actually exists, preventing any chance of inconsistent state or panics if modal_open is ever true without a draft. The cloned draft is used read‑only for this frame, and all mutations still flow through messages, preserving the MVU contract. This is a clean, Picard‑approved refinement.

Also applies to: 1497-1522
src/models/attachment.rs (2)
27-46: Clarify sanitized_name precondition and link to the validator.

This documentation is strong and sets the right expectations. To be perfectly clear: uniqueness is a precondition here, not enforced by the constructor itself. I recommend explicitly pointing callers to assert_unique_sanitized_names as the companion invariant check.

For example:
-/// The caller must provide a name that is already filesystem-safe and
-/// unique within the archive.
+/// The caller must provide a name that is already filesystem-safe and
+/// unique within the archive.
+///
+/// Use [`assert_unique_sanitized_names`] when assembling an archive to
+/// validate that this uniqueness invariant holds.
This keeps the model aligned with the Attachments UI’s sanitize/dedupe behavior. Based on learnings, this will make the data flow more obvious between src/models/attachment.rs and the attachments panel.

64-93: Duplicate-name detection is correct; review callers and consider avoiding the extra clone.

The HashSet-based duplicate check behaves as advertised: the first occurrence is accepted, subsequent identical sanitized_name values correctly trigger an error with a clear message. This is a welcome tightening of invariants.

Two follow-ups I recommend:

Call-site behavior. Previously this function always returned Ok(()); now duplicates surface as Err. Please review the call sites (in particular anything feeding the Attachments UI or archive creation) to ensure they handle this error path appropriately—e.g., mapping it to a user-visible warning rather than an unexpected panic. Based on learnings, that keeps the Attachments panel’s warning story coherent.

Optional micro-optimization. To avoid allocating cloned Strings solely for set membership, you could store &str in the set:
-pub fn assert_unique_sanitized_names(attachments: &[Attachment]) -> Result<()> {
-    let mut seen = HashSet::new();
-    for att in attachments {
-        if !seen.insert(att.sanitized_name.clone()) {
+pub fn assert_unique_sanitized_names(attachments: &[Attachment]) -> Result<()> {
+    let mut seen: HashSet<&str> = HashSet::new();
+    for att in attachments {
+        if !seen.insert(&att.sanitized_name) {
             return Err(anyhow!(
                 "Duplicate attachment filename in archive: {}",
                 att.sanitized_name
             ));
         }
     }
     Ok(())
 }
The existing implementation is entirely acceptable; this adjustment is merely a refinement.
src/models/keywords.rs (1)
58-70: Optional: use a set for normalization to simplify intent and scale better.

The current normalize implementation is correct and matches the documented behavior, but it performs an O(n²) scan via seen.contains on a Vec<String>. If keyword lists grow, you could simplify the logic and improve scaling with a HashSet:
-use super::*;
+use std::collections::HashSet;
+
 fn normalize(&mut self) {
-    // Dedup case-insensitively while preserving original casing of first occurrence.
-    let mut seen = Vec::<String>::new();
-    self.items.retain(|kw| {
-        let lower = kw.to_ascii_lowercase();
-        if seen.contains(&lower) {
-            false
-        } else {
-            seen.push(lower);
-            true
-        }
-    });
+    // Dedup case-insensitively while preserving original casing of first occurrence.
+    let mut seen = HashSet::new();
+    self.items.retain(|kw| seen.insert(kw.to_ascii_lowercase()));
 }
Functionally identical, but slightly clearer in its intent. The existing approach is acceptable if keyword counts are expected to remain small; treat this as a refinement to consider rather than a requirement.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55bc00f and ec2cfaa.

📒 Files selected for processing (11)

.github/workflows/release.yml (1 hunks)
src/app/mod.rs (1 hunks)
src/logic/eln.rs (4 hunks)
src/models/attachment.rs (2 hunks)
src/models/extra_fields.rs (4 hunks)
src/models/keywords.rs (2 hunks)
src/mvu/mod.rs (3 hunks)
src/ui/components/extra_fields.rs (28 hunks)
src/ui/mod.rs (0 hunks)
src/utils/hash.rs (1 hunks)
src/utils/sanitize_component.rs (1 hunks)

💤 Files with no reviewable changes (1)

src/ui/mod.rs

🧰 Additional context used

📓 Path-based instructions (8)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Rust 2024 crate; entry point at src/main.rs, which calls app::run() to start eframe/egui. SPDX headers on sources: SPDX-License-Identifier: MIT plus one or more SPDX-FileCopyrightText lines naming actual authors.
Use /// for public items and //! for module-level docs in Rustdoc; start with a one-line summary ending with a period.
Structure Rustdoc details with # headings (e.g., # Examples, # Errors, # Panics, # Safety, # Performance); include small runnable examples marked no_run/ignore when side effects exist.
Explain invariants, panics, and error cases explicitly in Rustdoc; prefer present tense and describe behavior, not intent.
Link related items in Rustdoc with intra-doc links like [TypeName] or [module::function]; disambiguate with full paths when needed.
Follow rustfmt defaults (4-space indent, trailing commas where appropriate); run cargo fmt before committing.
Prefer snake_case for functions/variables/files and CamelCase for types; keep module files small and focused.
Code comments: use sparingly to explain intent, invariants, or non-obvious control flow; avoid restating what the code already makes clear.
Name tests after behavior (e.g., submits_trimmed_input) and keep them deterministic.

Files:

src/app/mod.rs
src/models/extra_fields.rs
src/logic/eln.rs
src/mvu/mod.rs
src/utils/sanitize_component.rs
src/utils/hash.rs
src/ui/components/extra_fields.rs
src/models/keywords.rs
src/models/attachment.rs

src/app/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/app/ handles eframe bootstrap and font/theme setup.

Files:

src/app/mod.rs

src/models/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/models/ contains pure data/validation (attachment, keywords).

Files:

src/models/extra_fields.rs
src/models/keywords.rs
src/models/attachment.rs

src/logic/eln.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/logic/eln.rs: src/logic/eln.rs handles ELN RO-Crate build/write, metadata, suggested archive name. Conform to RO-Crate 1.2 and ELN File Format spec; use pre-sanitized names from attachments. No UI deps. Extra fields export: archives emit eLabFTW-style PropertyValue nodes with metadata blob.
For archive structure and metadata, align with the ELN File Format specification: https://raw.githubusercontent.com/TheELNConsortium/TheELNFileFormat/refs/heads/master/SPECIFICATION.md

Files:

src/logic/eln.rs

src/mvu/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Update layer should route messages in mvu::update, validate save requests, and enqueue Commands in run_command.

Files:

src/mvu/mod.rs

src/utils/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/utils/ contains helpers (sanitize_component, hash_file).

Files:

src/utils/sanitize_component.rs
src/utils/hash.rs

src/ui/components/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

View components should implement view(...) -> Vec<Msg> in ui/components/*; ui::ElnPackApp composes and wraps into mvu::Msg.

Files:

src/ui/components/extra_fields.rs

src/ui/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/ui/**/*.rs: UI components in src/ui/ and src/ui/components/* stay side-effect free; they emit messages only.
Phosphor icons: the font is registered in src/main.rs; use egui_phosphor::regular::NAME (via RichText or button labels) instead of embedding SVGs. Keep icon+text buttons short and reuse helpers already in src/ui.rs.
src/ui/ handles UI composition and screens; collects component messages and feeds them into mvu::update/run_command. Save flow: opens file dialog, dispatches Msg::SaveRequested; kernel validates.
File dialog operations use rfd crate for cross-platform native dialogs.

Files:

src/ui/components/extra_fields.rs

🧠 Learnings (25)

📓 Common learnings

Learnt from: CR
Repo: Athemis/ELNPack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T09:56:40.111Z
Learning: Pull requests should describe the change, note any UI updates, and link related issues. Include screenshots or short screen recordings when modifying UI behavior.

Learnt from: CR
Repo: Athemis/ELNPack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T09:56:40.111Z
Learning: Keep commits scoped and reviewable; avoid bundling formatting-only changes with behavior changes.